Skip to content

Fix formal review findings across all 9 review-sets#175

Merged
Malcolmnixon merged 3 commits intomainfrom
copilot/identify-review-sets-and-fix-failures
Apr 1, 2026
Merged

Fix formal review findings across all 9 review-sets#175
Malcolmnixon merged 3 commits intomainfrom
copilot/identify-review-sets-and-fix-failures

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 1, 2026

Pull Request

Description

Performed formal reviews for all 9 review-sets defined in .reviewmark.yaml and fixed the identified issues.

Review Sets Processed

Review Set Status
SpdxTool-System Issues found and fixed
SpdxTool-Targets-System Issues found and fixed
SpdxTool-Design Issues found and fixed
SpdxTool-AllRequirements No issues
SpdxTool-Context Issues found and fixed
SpdxTool-Commands Issues found and fixed
SpdxTool-SelfTest No issues
SpdxTool-Utility Issues found and fixed
SpdxTool-OTS Noted (MSTest OTS requirement logic concern, no code change needed)

Code Fixes

  • RenameId.cs: Corrected wrong error message — 'spdx''old' for the missing old input parameter
  • RenameId.cs: Removed unreachable dead code (redundant oldId == newId check after an early return that already handles this case)
  • Validate.cs: Corrected wrong command name in error message — 'to-markdown''validate'
  • Context.cs: Replaced int.Parse with int.TryParse so invalid --depth values throw InvalidOperationException (handled by Program.Main) instead of an unhandled FormatException
  • Context.cs: Wrapped StreamWriter constructor in separate catch blocks for UnauthorizedAccessException, ArgumentException, NotSupportedException, and IOException, each producing a distinct descriptive InvalidOperationException message
  • Runner.cs, DotnetRunner.cs, Query.cs: Fixed potential stdout/stderr deadlock — read both streams concurrently via ReadToEndAsync + Task.WaitAll instead of sequentially with ReadToEnd
  • Runner.cs, DotnetRunner.cs: Added using var process to ensure the Process instance and its redirected streams are properly disposed after each test run

Documentation Fixes

  • introduction.md: Fixed software structure diagram — moved RelationshipDirection.cs and SpdxHelpers.cs from the Utility (Subsystem) section to a correct Spdx (Unit Group) section
  • spdxtool-system.md + commands/commands.md: Corrected class name CommandRegistryCommandsRegistry throughout (to match the actual implementation)
  • spdxtool-targets-system.md: Corrected MSBuild property name SpdxToolWorkflowSpdxWorkflowFile (to match the actual targets file)
  • utility.md: Corrected RelationshipDirection enum values — Forward/Reverse/BothParent/Child/Sibling (to match the actual implementation)

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code quality improvement

Related Issues

Pre-Submission Checklist

Before submitting this pull request, ensure you have completed the following:

Build and Test

  • Code builds successfully: dotnet build --configuration Release
  • All unit tests pass: dotnet test --configuration Release
  • Self-validation tests pass:
    dotnet run --project src/DemaConsulting.SpdxTool --configuration Release --framework net10.0
    --no-build -- --validate
  • Code produces zero warnings

Code Quality

  • Code formatting is correct: dotnet format --verify-no-changes
  • New code has appropriate XML documentation comments
  • Static analyzer warnings have been addressed

Quality Checks

Please run the following checks before submitting:

  • All linters pass: ./lint.sh (Unix/macOS) or cmd /c lint.bat / ./lint.bat (Windows)

Testing

  • Added unit tests for new functionality
  • Updated existing tests if behavior changed
  • All tests follow the AAA (Arrange, Act, Assert) pattern
  • Test coverage is maintained or improved

Documentation

  • Updated README.md (if applicable)
  • Updated ARCHITECTURE.md (if applicable)
  • Added code examples for new features (if applicable)
  • Updated requirements.yaml (if applicable)

Additional Notes

The --depth and --log exception-handling fixes align Context.Create with the documented /// <exception cref="InvalidOperationException"> contract, so callers (including Program.Main) already handle these cases correctly without displaying raw stack traces to the user.

The deadlock fix in Runner.cs / DotnetRunner.cs / Query.cs is latent but real — it manifests only when child process stderr output fills its OS buffer while the parent is blocked draining stdout.

The using var process fix in Runner.cs and DotnetRunner.cs ensures OS handles and redirected stream resources are released promptly, preventing handle leaks across long test runs.

Copilot AI and others added 2 commits April 1, 2026 22:49
Agent-Logs-Url: https://github.com/demaconsulting/SpdxTool/sessions/7e5da612-1917-415a-9035-a05226aa5faf

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
…feedback

Agent-Logs-Url: https://github.com/demaconsulting/SpdxTool/sessions/7e5da612-1917-415a-9035-a05226aa5faf

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Copilot AI requested a review from Malcolmnixon April 1, 2026 22:54
@Malcolmnixon Malcolmnixon marked this pull request as ready for review April 1, 2026 23:13
Copilot AI review requested due to automatic review settings April 1, 2026 23:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses issues found during formal reviews across multiple review-sets by fixing incorrect/unhelpful error handling in CLI code, preventing potential process I/O deadlocks, and aligning design documentation with the current implementation.

Changes:

  • Improved CLI robustness and correctness (better error messages; safer --depth parsing; log-file creation failures mapped to InvalidOperationException).
  • Prevented potential stdout/stderr deadlocks by reading both redirected streams concurrently in runners and query.
  • Updated design documentation to match actual type/property names and the current system structure.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
test/DemaConsulting.SpdxTool.Tests/Runner.cs Read stdout/stderr concurrently to avoid deadlock when capturing process output in tests.
test/DemaConsulting.SpdxTool.Targets.Tests/DotnetRunner.cs Read stdout/stderr concurrently to avoid deadlock for dotnet integration test runs.
src/DemaConsulting.SpdxTool/Context.cs Safer --depth parsing and improved log writer creation error handling.
src/DemaConsulting.SpdxTool/Commands/Validate.cs Corrected command name in a YAML exception message.
src/DemaConsulting.SpdxTool/Commands/RenameId.cs Corrected YAML exception message and removed unreachable dead code.
src/DemaConsulting.SpdxTool/Commands/Query.cs Read stdout/stderr concurrently to avoid deadlock when querying external program output.
docs/design/utility.md Updated RelationshipDirection enum values to match implementation.
docs/design/spdxtool-targets-system.md Updated MSBuild property name to SpdxWorkflowFile to match targets.
docs/design/spdxtool-system.md Updated registry naming to CommandsRegistry to match implementation.
docs/design/introduction.md Corrected software structure diagram placement for SPDX-related units.
docs/design/commands/commands.md Updated references from CommandRegistry to CommandsRegistry.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Malcolmnixon
Copy link
Copy Markdown
Member

@copilot apply changes based on the comments in this thread

…Process disposal

Agent-Logs-Url: https://github.com/demaconsulting/SpdxTool/sessions/d6c7890b-568e-452b-a0be-0390962bb82c

Co-authored-by: Malcolmnixon <1863707+Malcolmnixon@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants